-
Notifications
You must be signed in to change notification settings - Fork 450
chore: Add a DA service unit test #3423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…/com.unity.netcode.gameobjects into chore/rust-unit-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only 1 or 2 questions and a minor suggestion on wording for API documentation.
Otherwise, very nice job:
- Cleaning up various areas within the NetcodeIntegrationTest.
- Improved the "readability" in many places.
- Added a more standard way to obtain various NetworkManager instances.
- Cleaning up various integration tests
- Improved the "readability" in many places.
- Created a more uniform standard "flow" and naming.
- Adding UseCMBService to tests that still needed conversion along with comments.
- Removing m_EnableVerboseDebug to reduce over-all test runner log size.
🥇 💯
com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs
Outdated
Show resolved
Hide resolved
com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs
Show resolved
Hide resolved
....netcode.gameobjects/Tests/Runtime/DistributedAuthority/NetworkClientAndPlayerObjectTests.cs
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/OwnershipPermissionsTests.cs
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs
Show resolved
Hide resolved
com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariable/NetworkVariableCollectionsTests.cs
Show resolved
Hide resolved
testproject/Assets/Tests/Runtime/NetworkSceneManager/InScenePlacedNetworkObjectTests.cs
Show resolved
Hide resolved
This is an attempt to fix an issue where Mac OSX is frequently failing the InterpolationStopAndStartMotionTest. Based on the error, I am thinking (hard to tell since it doesn't replicate locally in-editor nor stand alone) that it could be due to a Mac VM running slower than expected (randomly) which could cause the network tick event to trigger more than once in a single frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!
# Set this to ensure the DA codec tests will fail if they cannot connect to the echo-server | ||
# The default is to ignore the codec tests if the echo-server fails to connect | ||
ENSURE_CODEC_TESTS: "true" | ||
USE_CMB_SERVICE: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where we are using it 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's inside the UseCMBService()
function inside NetcodeIntegrationTest.cs
(here)
## This PR Includes ### Initial setup changes - Minor improvements to the initial CMB server detection and adjustments to the initial configuration order of operations. - Session owner now starts and connects ahead of the other clients. ### Ignoring tests This PR includes updates that will ignore any test that: - Uses a client-server network topology - _No point in re-running these as they will have already run multiple times on multiple platforms before testing against the CMB server._ - Has overridden UseCmbService and is returning false. - _The test has yet to be converted and so no point in even attempting to run it._ - _This also includes the `TODO: [CmbServiceTests]` to help track all tests that need to be converted or reviewed to determine if they need to be converted._ - Is not derived from NetcodeIntegrationTest. - _Many of these (not all) don't need to run again, but they all have the `TODO: [CmbServiceTests]` to help track all tests that need to be converted or not._ - Does not need to be tested against the CMB Server (i.e. a test for some kind of functionality without actually starting a session and the like). _(This helps to reduce the time to complete the CMB Server integration tests)_ ### Marking/Tracking "for review" tests Assuring that every test that needs to be reviewed includes the `TODO: [CmbServiceTests]` using of the two ways: For classes that derive from `NetcodeIntegrationTest` the existing pattern used in #3423 was applied: ``` // TODO: [CmbServiceTests] Adapt to run with the service protected override bool UseCMBService() { return false; } ``` For classes that **do not** derive from `NetcodeIntegrationTest`, an added internal helper method was used: ``` [OneTimeSetUp] public void OneTimeSetup() { // TODO: [CmbServiceTests] if this test is deemed needed to test against the CMB server then update this test. NetcodeIntegrationTestHelpers.IgnoreIfServiceEnviromentVariableSet(); } ``` _(This PR includes some tests that includes one of the two script segments but was was reviewed and determined it didn't need to be run against a CMB server)_ ## Fixes Some issues were exposed in regards to the `NetworkClient` not being set to approved on all clients relative to each local `NetworkManager` instance when using the distributed authority network topology and connecting to a CMB server. ## Changelog NA - All internal testing and/or specific to testing. ## Testing and Documentation - Includes integration test adjustments. - Includes `NetcodeIntegrationTest` related adjustments. - Includes `NetcodeIntegrationTestHelpers` related adjustments. - No documentation changes or additions were necessary. ## Backport Does not require a backport since these changes are specific to updates in #3423 and over-all distributed authority integration testing.
MTT-11550
Changelog
Testing and Documentation
Backport
No backport applicable - DA only